ABI-compatible and fixed-capacity vectors #5
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
pawelrutkaq
left a comment
pawelrutkaq
left a comment
There was a problem hiding this comment.
Once we finihs I would ping this to baselibs team to have a look if interested
.vscode/settings.json
Outdated
| "editor.defaultFormatter": "rust-lang.rust-analyzer", | ||
| "editor.formatOnSave": true, | ||
| "editor.rulers": [ | ||
| 100 |
There was a problem hiding this comment.
this shall be aligned with rustfmt
| @@ -0,0 +1,20 @@ | |||
| // ******************************************************************************* | |||
There was a problem hiding this comment.
should we call this folder abi_datatypes _cd is bit misterious.
There was a problem hiding this comment.
now we have to change that and move generic part to seprate thing, and live here only ABI types, or vice versa or ?
src/abi_cd/BUILD
Outdated
rustfmt.toml
Outdated
|
|
||
|
|
||
| match_block_trailing_comma = true | ||
| max_width = 100 |
There was a problem hiding this comment.
shall this be 120-150. 100 is bit low.
There was a problem hiding this comment.
I prefer 100, because I like to put two tabs side-by-side. However, I've changed it back to 150 – maybe I need a bigger screen 😉
src/abi_datatypes/Cargo.toml
Outdated
| @@ -0,0 +1,27 @@ | |||
| [package] | |||
There was a problem hiding this comment.
Baselibs Rust shall be dependent less.
There was a problem hiding this comment.
and I would keep effort low on cargo so would derive everything from workspace (desc, versions etc)
There was a problem hiding this comment.
That's not possible, I need to import those crates to be able to derive PlacementDefault and Reloc on the types.
There was a problem hiding this comment.
We have to do it differently. We will import baselib_rust into comm API and implement Reloc there. is that ok ? We will also define placement trait in there so we can implement it there.
src/abi_cd/src/vec.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn flags() { |
There was a problem hiding this comment.
hard to get what this name means
There was a problem hiding this comment.
I've renamed it to abi/fixed_vec_is_full_and_is_empty()
src/abi_cd/src/queue.rs
Outdated
| len: u32, | ||
| /// The index of the next element to be popped. | ||
| read_pos: u32, | ||
| elements: [MaybeUninit<T>; CAPACITY], |
There was a problem hiding this comment.
Wouldn't replacing len and elements with AbiVec be better?
There was a problem hiding this comment.
In an AbiVec, the valid elements are always in slots 0...len-1, but this typically isn't the case for AbiQueue: in a queue, they could be in slots 1...len or 2...len+1, and they might even be in two separate ranges at the beginning and the end of the slots.
Also, replacing len and elements with AbiVec would change the memory layout of the struct.
src/abi_cd/src/queue.rs
Outdated
| (first, second) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
What is semantic in those calls? We cannot easilly say where beggining or end is. This opens a question whether we shall expose this api at ASIL-B level..
There was a problem hiding this comment.
This method is analogous to VecDeque::as_slices. The purpose is to provide very efficient access to all queued elements.
I could write a more detailed description into the docs of this method.
src/abi_cd/src/queue.rs
Outdated
| /// | ||
| /// * `start <= end <= CAPACITY` must hold | ||
| /// * all storage slots in the range `start..end` must be valid | ||
| const unsafe fn subslice(&self, start: usize, end: usize) -> *const [T] { |
There was a problem hiding this comment.
move private to bottom of impl block.
src/abi_cd/src/queue.rs
Outdated
| @@ -0,0 +1,398 @@ | |||
| // ******************************************************************************* | |||
There was a problem hiding this comment.
I will wait till we fix vector as his have same questions ;)
awillenbuecher-xq-tec
force-pushed
the
adwi_abivec
branch
from
2910c0d to
ceefd1b
Compare
src/abi_datatypes/src/storage.rs
Outdated
| /// # Safety | ||
| /// | ||
| /// `index < self.capacity()` must hold. | ||
| unsafe fn element(&self, index: u32) -> &MaybeUninit<T>; |
There was a problem hiding this comment.
this does not need to be unsafe or ?
There was a problem hiding this comment.
This must be unsafe, because if index >= capacity, this will result in an out-of-bounds array access.
src/abi_datatypes/src/storage.rs
Outdated
| /// # Safety | ||
| /// | ||
| /// `index < self.capacity()` must hold. | ||
| unsafe fn element_mut(&mut self, index: u32) -> &mut MaybeUninit<T>; |
There was a problem hiding this comment.
see above
src/abi_datatypes/src/storage.rs
Outdated
| impl<T, const CAPACITY: usize> Inline<T, CAPACITY> { | ||
| // Compile-time check. This condition _must_ be referenced in every function that depends on it, | ||
| // otherwise it will be removed during monomorphization. | ||
| const CHECK_CAPACITY: () = assert!(0 < CAPACITY && CAPACITY <= (u32::MAX as usize)); |
There was a problem hiding this comment.
should we limit elems to u32 or use u64 ?
There was a problem hiding this comment.
I don't there's a realistic use case for a having more than 4.2 billion elements in a single ABI compatible container, is there? Similar for the non-ABI, fixed capacity containers – even with u8 as elements, that would require reserving more than 4 GiB of memory just for one vector in your embedded system, and it quickly becomes worse when you want to store anything "interesting" in it 😉
src/abi_datatypes/src/storage.rs
Outdated
| debug_assert!(index < self.capacity); | ||
| let index = index as usize; | ||
| // SAFETY: `index` is in-bounds of the memory allocation, as per the pre-condition on the trait method. | ||
| unsafe { self.elements.add(index).cast::<MaybeUninit<T>>().as_ref() } |
There was a problem hiding this comment.
maybe add that maybeUninit is repr transparent thats why casting form T is ok ?
src/abi_datatypes/src/storage.rs
Outdated
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
we need UT for those here.
| @@ -0,0 +1,20 @@ | |||
| // ******************************************************************************* | |||
There was a problem hiding this comment.
now we have to change that and move generic part to seprate thing, and live here only ABI types, or vice versa or ?
src/abi_datatypes/src/vec.rs
Outdated
| /// | ||
| /// The vector can hold between 0 and `CAPACITY` elements, and behaves similarly to Rust's `Vec`, | ||
| /// except that it allocates memory immediately on construction, and can't shrink or grow. | ||
| pub struct FixedVec<T> { |
There was a problem hiding this comment.
RuntimFixedVec
NonRelocVec
or as it is ;)
There was a problem hiding this comment.
Then I'll just leave it at FixedVec.
src/abi_datatypes/src/vec.rs
Outdated
|
|
||
| impl<T> Drop for FixedVec<T> { | ||
| fn drop(&mut self) { | ||
| if needs_drop::<T>() { |
There was a problem hiding this comment.
redudant, this is in clear() already.
src/abi_datatypes/BUILD
Outdated
| load("@rules_rust//rust:defs.bzl", "rust_library", "rust_test") | ||
|
|
||
| rust_library( | ||
| name = "abi-types-common", |
There was a problem hiding this comment.
i think bazel target shall be underscore. and you may use crate_name to define it rust wise if you need
awillenbuecher-xq-tec
force-pushed
the
adwi_abivec
branch
2 times, most recently
from
9f1d3a3 to
e630a04
Compare
awillenbuecher-xq-tec
force-pushed
the
adwi_abivec
branch
from
e630a04 to
e59b4b6
Compare
awillenbuecher-xq-tec
changed the title
| use crate::storage::Storage; | ||
|
|
||
| #[repr(C)] | ||
| pub struct GenericVec<T, S: Storage<T>> { |
There was a problem hiding this comment.
Should we provide somewhere alias for FixedSizeVec ? The users may want to also use compile time vec too.
src/containers/BUILD
Outdated
| name = "containers", | ||
| srcs = glob(["**/*.rs"]), | ||
| edition = "2024", | ||
| version = "0.1.0", |
There was a problem hiding this comment.
this is not needed for bazel as we anyway version bazel module only
awillenbuecher-xq-tec
force-pushed
the
adwi_abivec
branch
from
e59b4b6 to
91a604b
Compare
awillenbuecher-xq-tec
changed the title
3 participants
This PR adds two data structures to the Rust base libs: an ABI-compatible and a fixed-capacity vector. Refer to the feature request for more information about ABI compatibility.
Notes for Reviewer
Pre-Review Checklist for the PR Author
Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References